Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FISH-1274 Vulnerability in Metro's WSDL Code Importing/Parsing - Remote Code Execution #5198

Merged
merged 3 commits into from
Apr 21, 2021

Conversation

Pandrex247
Copy link
Member

@Pandrex247 Pandrex247 commented Apr 16, 2021

Description

This is a bug fix.

The Webservice Tester servlet had a possible Host Header attack vector, allowing for a malicious WSDL to potentially be imported. Combined with a code injection vulnerability in Metro, this allowed an attacker to potentially run their own code on a machine. This PR adds extra validation to the tester servlet to ensure the host header is a valid one, and also adds extra validation to Metro itself to help ensure that any replacement being done is a valid variable to help prevent code injection.

Important Info

I'm not entirely sure if checking if the variable replacement is a valid java variable is in accordance with the spec (I can't find anything saying one way or the other), but I inferred it to be true given that Metro is already validating that the variable being replaced isn't a Java keyword.

Dependant PRs

Tags haven't been made yet as I'm trying to get this tested.

Blockers

Metro-JAX-WS has been compiled and deployed to https://nexus.payara.fish/repository/payara-artifacts, for some reason however when I try to deploy Metro-WSIT I get a 401 on the org.glassfish.metro:webservices-osgi:jar:2.4.4.payara-p1 artifact - numerous other artifacts deploy successfully, for some reason it hates this one.

Resolved.

Testing

Test outlined in the issue: https://payara.atlassian.net/browse/FISH-1274
It now rejects the request with an error.

Also tested using payara5/glassfish/bin/wsimport and the evil payload.wsdl directly, since fixing the Host Header attack prevents the wsimport from happening in the first place.
It now throws an error during import so that no code gets compiled and written out - without the fix it would import "successfully", with the evil code injection happening (with breakpoints you would see it write out the java class with the evil code injected)

New tests

None yet.

Testing Environment

WSL OpenSUSE Leap, Zulu JDK 8 and 11

Documentation

N/A

Notes for Reviewers

Anyone who knows JAX-WS / JAXB appreciated!

I'd also appreciate someone to double-check my regex:

@Pandrex247 Pandrex247 requested review from pdudits and fturizo April 16, 2021 11:22
@Pandrex247
Copy link
Member Author

Jenkins test please

@Pandrex247
Copy link
Member Author

Looks like there are issues with metro wsit 2.4.4 / metro-jax-ws 2.3.3, as the webservices TCK gets failures when they're used with Payara (with and without my changes).
Will try again with my changes on 2.4.3 / 2.3.2 and see if it passes

@Pandrex247 Pandrex247 added the PR: DO NOT MERGE Don't merge PR until further notice label Apr 16, 2021
@@ -145,7 +145,6 @@
<hibernate.validator-cdi.version>6.1.5.Final</hibernate.validator-cdi.version>
<jakarta.el.version>3.0.3.payara-p4</jakarta.el.version>
<jakarta.el-api.version>3.0.3.payara-p4</jakarta.el-api.version>
<jaxb-api.version>2.3.2</jaxb-api.version>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate

@Pandrex247 Pandrex247 removed the PR: DO NOT MERGE Don't merge PR until further notice label Apr 16, 2021
@Pandrex247
Copy link
Member Author

Pandrex247 commented Apr 16, 2021

Metro JAX-WS 2.3.2, and WSIT 2.4.3, with my added changes, don't appear to be failing the subset of the TCK I was testing against. Updated PR and removed do not merge label

@Pandrex247
Copy link
Member Author

Jenkins test please

1 similar comment
@Pandrex247
Copy link
Member Author

Jenkins test please

// Split off for unit testing
protected static boolean checkValidDnsName(String decodedServerName) {
// NOTE: HttpServletRequest.getServerName strips the port so no need to check
return decodedServerName.matches("((?!-)[\\w-]{1,63}(?<!-)(\\.|$))+");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that rules depend on some local authority. What it will do with IDN?
https://en.wikipedia.org/wiki/Internationalized_domain_name
https://en.wikipedia.org/wiki/Punycode
https://docs.oracle.com/javase/8/docs/api/java/net/IDN.html
I am not sure if the UrlDecoder is the right one for this ...

EDIT: HTTP request to an url using my custom hostname řízeček:8080 passed, so at least this is an example of an ugly header. Translation was done by the browser, but I had to use the translated value in /etc/hosts
image

Copy link
Member Author

@Pandrex247 Pandrex247 Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UrlDecoder is used elsewhere in this code, which is where I took it from.
Patrik and myself looked at IDNs - RFC for DNS Host Header states something along the lines (can't remember exactly) of TLDR: ASCII characters only.

@Pandrex247 Pandrex247 merged commit 19cec93 into payara:master Apr 21, 2021
JamesHillyard pushed a commit to JamesHillyard/Payara that referenced this pull request Oct 28, 2021
FISH-1274 Vulnerability in Metro's WSDL Code Importing/Parsing - Remote Code Execution
@Pandrex247 Pandrex247 deleted the FISH-1274-Comm branch March 29, 2022 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants